[Feature] Component Implementation - Button #34

narin · 2023-11-08T19:18:25Z

Ticket department-of-veterans-affairs/va-mobile-app#6870

Description of Change

Implemented VAButton with the following variants
- Base
- BaseSecondary
- Primary
- Secondary
- Destructive
- White (this is a one-off variant used only on the VA Mobile App's login screen)
Tokenized all colors used (aside from White variant onPress)
- Note: Some tokens either had duplicates in VADS tokens OR were missing completely
- For those that had duplicates between VADS's color.json and uswds.json, I used the VADS naming
- Missing color tokens were found on USWDS's site
Published v0.2.0 of tokens to NPM
Added Storybook stories for each variant
Added unit tests for rendering, onPress, and light mode variants

Notable differences from original VAButton

Simplified to use Pressable's built in pressed state instead of keeping track of it in a useState hook
Use enum for button variants instead of constants + type
Removed props
- hideBorder (this is instead built into the variant styling)
- minHeight (not used anywhere in the app)
- iconProps (not part of VADS)
- disabled (VADS discourages disabled state)
- disabledText (VADS discourages disabled state)
- accessibilityState (not used and not needed without a disabled state)

Breaking changes

Appointment details screen currently uses a disabled button which is not supported by our component library. We will need to work with health PM to create a ticket.

Testing Packages

components-v0.1.1-alpha.0
components-v0.1.1-alpha.1
components-v0.1.1-alpha.2
components-v0.1.1-alpha.3
components-v0.1.1-alpha.4
components-v0.1.1-alpha.5
components-v0.1.1-alpha.6
components-v0.1.1-alpha.7
components-v0.1.1-alpha.8
components-v0.1.1-alpha.9
components-v0.1.1-alpha.10
components-v0.1.1-alpha.11
components-v0.1.1-alpha.12
components-v0.1.1-alpha.13
components-v0.1.1-alpha.14
components-v0.1.1-alpha.15
tokens-v0.1.13-alpha.0
tokens-v0.1.13-alpha.1
tokens-v0.1.13-alpha.2
tokens-v0.1.13-alpha.3
tokens-v0.2.1-alpha.1

Screenshots/Video

iOS

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2023-11-08.at.10.57.06.mp4

Android

Screen.Recording.2023-11-08.at.11.04.02.AM.mov

Web

Screen.Recording.2023-11-08.at.11.20.11.AM.mov

White variant usage in mobile app

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2023-11-16.at.12.00.08.mp4

Testing

Tested on iOS
Tested on Android
Tested on Web

PR Checklist

Code reviewer validation:

General
- PR is connected to ticket(s)
- Acceptance criteria:
  - All satisfied or
  - Documented reason for not being performed or
  - Split to separate ticket and ticket is linked by relevant AC(s)
- Above PR sections adequately filled out
- If any breaking changes, in accordance with the pre-1.0.0 versioning guidelines: a CU ticket has been created for the VA Mobile App detailing necessary adjustments with the package version that will be published by this ticket
Code
- Tests are included if appropriate (or split to separate ticket)
- New functions have proper TSDoc annotations

Publish

If changes warrant a new version per the versioning guidelines and the PR is approved and ready to merge:

Merge main into branch
Publish new version
Merge branch to main

…affairs/va-mobile-library into 6870-nr-vabutton

TimRoe · 2023-11-14T17:25:53Z

packages/components/src/components/Button/Button.stories.tsx

+  },
+}
+
+export const Primary: Story = {


Nitpicky so don't feel strongly, but if there's a simple way within the Doc section to bump Primary to be at the top instead of Base that would probably make more sense.

Could not figure out a way to sort these properly. According to Storybook docs, they're supposed to be sorted by the way they are written in code but I'm not finding that to be the case. There's ways to set the manually, but we'd have to list out each story in an array which will be a bit cumbersome to do everytime we add a new story.

Yeah, initially tried just moving it to the top and was disappointed to see the sort didn't change. Oh well.

packages/components/src/components/Button/Button.tsx

TimRoe · 2023-11-15T09:43:16Z

Update:
Had a fresh idea about how to fix webStorybookColorScheme issues. Followed it through, seemed good, and should fix the package in the app (didn't test to verify) since we now no longer import storybook-dark-mode directly at all and, correspondingly, I was able to move that package to a devDependency where it belongs so it won't be part of the published package.

Still didn't look at the updated unit tests yet, but everything else looks good (including testing iOS/Android/Web) except some minor comments.

packages/components/src/components/Button/Button.test.tsx

TimRoe · 2023-11-15T19:36:44Z

packages/components/src/components/Button/Button.test.tsx

+
+  it('should render Primary pressed variant', async () => {
+    component = render(
+      <Button label={label} onPress={onPressSpy} testOnlyPressed={true} />,


Was testOnlyPressed also needed for detox or anything? If not, wonder if there's a way to check the pressed state without needing to add a prop to force it--seems like there should be. Taking a very brief glance at the testing library docs, I see fireEvent.keyDown exists and wonder if there's similarly a press-and-hold function that would also yield the pressed styles. Or maybe even that one works even though it's not a "key" for us.

Was initially trying to use fireEvent(button, 'pressIn') which is the RN equivalent, but it was not working to render the pressed styles. testOnlyPressed was the only one I had success with, but it's not ideal since it has to be exposed as a prop.

TimRoe · 2023-11-15T19:47:37Z

packages/components/src/components/Button/Button.test.tsx

+    const { backgroundColor, borderWidth } = button.props.style
+    const { color } = text.props.style
+
+    expect(backgroundColor).toEqual(


Thought about mentioning it, but you'd already ran with integrating into jest tests which is fine for now.

However: wonder if we should explore other testing options (maybe jest has extensions for it too?) that do an actual UI snapshot to test styles instead of programmatically doing it which is a bit forced. I know at some RN virtual conference back in like 2020 or 2021 there was a presentation on a visual testing library that would just have pictures of before and compare to the after and highlight any differences down to the pixel (intentional or not) to confirm. Probably beyond the scope of this ticket anyway, but maybe worth a spike to explore since that seems a much more intuitive way to unit tests styles if it wasn't too much of a lift. In theory, it'd probably be much quicker/less tedious as well if we committed to style testing every component which likely does make sense for a design system.

Created spike ticket to explore this: https://github.com/department-of-veterans-affairs/va-mobile-app/issues/7319

TimRoe

Dropping a re-request changes to denote the end of the 2nd pass. Not sure any of the new comments absolutely need changes, but I think some tweaks would be good. I think the only truly outstanding thing is confirming it works in the app now--which the commit I added should hopefully allow.

narin · 2023-11-16T18:11:12Z

Update: Had a fresh idea about how to fix webStorybookColorScheme issues. Followed it through, seemed good, and should fix the package in the app (didn't test to verify) since we now no longer import storybook-dark-mode directly at all and, correspondingly, I was able to move that package to a devDependency where it belongs so it won't be part of the published package.

Still didn't look at the updated unit tests yet, but everything else looks good (including testing iOS/Android/Web) except some minor comments.

App runs and I can import the button successfuly, but TypeScript is giving complaints about the web related code:

[1] node_modules/@department-of-veterans-affairs/mobile-component-library/src/utils/index.tsx(7,10): error TS2305: Module '"react"' has no exported member 'useSyncExternalStore'.
[1] node_modules/@department-of-veterans-affairs/mobile-component-library/src/utils/index.tsx(17,31): error TS7006: Parameter 'callback' implicitly has an 'any' type.
[1] node_modules/@department-of-veterans-affairs/mobile-component-library/src/utils/index.tsx(18,5): error TS2304: Cannot find name 'window'.
[1] node_modules/@department-of-veterans-affairs/mobile-component-library/src/utils/index.tsx(19,18): error TS2304: Cannot find name 'window'.
[1] node_modules/@department-of-veterans-affairs/mobile-component-library/src/utils/index.tsx(21,26): error TS2304: Cannot find name 'window'.
[1] node_modules/@department-of-veterans-affairs/mobile-component-library/src/utils/index.tsx(21,39): error TS2304: Cannot find name 'window'.

…ackage

TimRoe · 2023-11-16T19:26:22Z

App runs and I can import the button successfuly, but TypeScript is giving complaints about the web related code:

Ugh! Added another commit to fix--adding listeners this way still seems to work with it abstracted out so was able to pull it out similarly to our Expo initialization which should remove those TS complaints because now the code won't exist in the NPM package.

I also changed from listening to the top body in favor of the parent body--because I think it would've been bugged when the iframe was within the doc site (since then it'd be listening to the doc site "body" in the HTML at the top level instead of the one "body" above it that the toggle would be switching the className between "light" and "dark". Fun that in normal circumstances there should only be a single "body" in a website, but within the doc site there's 3: the doc site's, storybook's, and the component's (or 3 layers I guess, more if you account for each iframe having 2 body's each).

TimRoe · 2023-11-16T19:37:26Z

Checking iOS I see my follow-up fix caused a require cycle. Should be fine, but will give us an annoying warning in iOS/Android.

I'd say we should move to attempt to implement ticket 7194 over resolving the require cycle by effectively making a duplicate structure for try-catching a different file than circling back to main.tsx.

narin · 2023-11-16T20:01:11Z

@TimRoe Tested within the app and errors are gone. Thanks!

Here is an example of the the button being used on the Login screen:

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2023-11-16.at.12.00.08.mp4

Address all other feedback. Ready for another round of review.

TimRoe

Approved, good to go through the publish checklist creating components package v0.2.0 and merge.

narin and others added 30 commits November 6, 2023 11:51

Finished all variants

bdde4f5

Refactor to use buttonType. Add white variant

4c5dca3

Add unit tests

73615cb

Add color tokens

6e4cac8

Version bump: tokens-v0.0.13-alpha.0

60208d3

Fix red color values

1265e91

Merge branch '6870-nr-vabutton' of github.com:department-of-veterans-…

4c6cb6a

…affairs/va-mobile-library into 6870-nr-vabutton

Add primary-darker color

20e4377

Version bump: tokens-v0.0.13-alpha.2

f8e311a

Add blue-vivid-30

cee8a51

Merge branch '6870-nr-vabutton' of github.com:department-of-veterans-…

3c414b1

…affairs/va-mobile-library into 6870-nr-vabutton

Version bump: tokens-v0.0.13-alpha.3

fba51b8

Version bump: tokens-v0.1.0

279daaf

Use tokenized colors

96971bf

Remove White story. Add a11yhint in Primary example

754f314

Version bump: components-v0.1.1-alpha.0

1662034

export VAButton in index.tsx

a1cd6ff

Version bump: components-v0.1.1-alpha.1

83ccad2

Export ButtonTypes

9dfcb9d

Merge branch '6870-nr-vabutton' of github.com:department-of-veterans-…

ea6e190

…affairs/va-mobile-library into 6870-nr-vabutton

Version bump: components-v0.1.1-alpha.2

3534b11

Refactor VAButton constants and types into enum

5644693

Version bump: components-v0.1.1-alpha.3

80aaac4

Add tsdoc

daaf163

Export VAButtonVariants

488e51c

Merge branch '6870-nr-vabutton' of github.com:department-of-veterans-…

92aafe4

…affairs/va-mobile-library into 6870-nr-vabutton

Version bump: components-v0.1.1-alpha.4

dd60e3e

Remove console log

36014a9

Merge branch '6870-nr-vabutton' of github.com:department-of-veterans-…

69a34b2

…affairs/va-mobile-library into 6870-nr-vabutton

Merge branch 'main' into 6870-nr-vabutton

2cdfa2c

TimRoe reviewed Nov 14, 2023

View reviewed changes

packages/components/src/components/Button/Button.tsx Show resolved Hide resolved

TimRoe reviewed Nov 14, 2023

View reviewed changes

packages/components/src/components/Button/Button.tsx Outdated Show resolved Hide resolved

webStorybookColorScheme fix, storybook-dark-mode devDependency'd

68b132b

TimRoe reviewed Nov 15, 2023

View reviewed changes

packages/components/src/components/Button/Button.test.tsx Outdated Show resolved Hide resolved

TimRoe reviewed Nov 15, 2023

View reviewed changes

TimRoe requested changes Nov 15, 2023

View reviewed changes

narin and others added 7 commits November 16, 2023 09:01

Set enum string values

747e61c

Update/add a11y labels and hints

9f6a783

Group variant tests

ce5d3a5

Grouped tests with default (primary) variant

c1ccd01

Cleaned up pressed logic

fe1be63

Combined imports

72ce74c

Version bump: components-v0.1.1-alpha.14

13fa90c

Further fixing webStorybookColorScheme, removing DOM stuff from npm p…

c78fb9c

…ackage

Version bump: components-v0.1.1-alpha.15

e27de2d

narin requested a review from TimRoe November 16, 2023 20:02

TimRoe approved these changes Nov 16, 2023

View reviewed changes

va-mobile-automation-robot and others added 2 commits November 16, 2023 20:43

Version bump: components-v0.2.0

3e74b2e

Trigger GitHub status checks

6e53452

narin merged commit 6dd70d6 into main Nov 16, 2023
5 checks passed

narin deleted the 6870-nr-vabutton branch November 27, 2023 20:55

narin changed the title ~~feature/6870-nr-vabutton~~ [Feature] Component Implementation - Button Nov 28, 2023

narin added the changelog label Nov 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Component Implementation - Button #34

[Feature] Component Implementation - Button #34

narin commented Nov 8, 2023 •

edited

Loading

TimRoe Nov 14, 2023

narin Nov 16, 2023

TimRoe Nov 16, 2023

TimRoe commented Nov 15, 2023

TimRoe Nov 15, 2023 •

edited

Loading

narin Nov 16, 2023

TimRoe Nov 15, 2023

narin Nov 16, 2023

TimRoe left a comment

narin commented Nov 16, 2023

TimRoe commented Nov 16, 2023 •

edited

Loading

TimRoe commented Nov 16, 2023

narin commented Nov 16, 2023

TimRoe left a comment

[Feature] Component Implementation - Button #34

[Feature] Component Implementation - Button #34

Conversation

narin commented Nov 8, 2023 • edited Loading

Description of Change

Notable differences from original VAButton

Breaking changes

Testing Packages

Screenshots/Video

iOS

Android

Web

White variant usage in mobile app

Testing

PR Checklist

Publish

TimRoe Nov 14, 2023

Choose a reason for hiding this comment

narin Nov 16, 2023

Choose a reason for hiding this comment

TimRoe Nov 16, 2023

Choose a reason for hiding this comment

TimRoe commented Nov 15, 2023

TimRoe Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

narin Nov 16, 2023

Choose a reason for hiding this comment

TimRoe Nov 15, 2023

Choose a reason for hiding this comment

narin Nov 16, 2023

Choose a reason for hiding this comment

TimRoe left a comment

Choose a reason for hiding this comment

narin commented Nov 16, 2023

TimRoe commented Nov 16, 2023 • edited Loading

TimRoe commented Nov 16, 2023

narin commented Nov 16, 2023

TimRoe left a comment

Choose a reason for hiding this comment

narin commented Nov 8, 2023 •

edited

Loading

TimRoe Nov 15, 2023 •

edited

Loading

TimRoe commented Nov 16, 2023 •

edited

Loading